Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add population forecast schema, example, and tests #69

Merged
merged 7 commits into from
Oct 17, 2022

Conversation

4lm
Copy link
Contributor

@4lm 4lm commented Oct 6, 2022

Hi @henhuy,

Please review and merge this JSON schema, example, and test implementation for the population popup on approval.

This first iteration of a JSON schema is just for the population popup. But I took the time to make it as generic as possible to be able to extend it to other popup types. I oriented myself on what we talked about in the past about good web app architecture, which is an intelligent backend and a dumb frontend. Therefore, the JSON response's custom description strings shall be created on the backend. Please look at the JSON example. This clarifies what I mean, what the backend should create, and how it should create it.

Please note, after merging, can you:

  1. create an API endpoint that returns (can be random) population values, like specified in the schema, for ONE municipality on:
/api/popup/?lookup=population&municipality=[1...20]
  1. create an API endpoint that returns (can be random) population values, like specified in the schema, for ALL municipalities on:
/api/popup/?lookup=population

Hint: Use the example JSON as a guide. The JSON schema is visually convoluted. I had to set the indent for JSON files in the editor config to 4 spaces* to not go crazy 😉

*The formatter always reformats it to 2 spaces on commit. So no worries here!

@4lm 4lm requested a review from henhuy October 6, 2022 17:33
@henhuy
Copy link
Collaborator

henhuy commented Oct 7, 2022

Thanks for adding schema and example for population results and related tests!
I have two questions:

  1. Does the schema for a single results (single municipality) looks different from current example.json? Or simply just have a single entry in series (aka len(series) == 1)? ;)
  2. Do schemas for other popups differ from example.json in population folder? Or can we agree on a default schema for all results? Or maybe a default schema, and differing schemas in case the schema does not fit for a special result? This would make development of the API much easier...

Some minor things I would change:

  • If we agree on a default schema, I would move up the schema.json one folder into folder schema.
  • I would like to remove init.py from metadata folders and keep it as a non-python/config-like structure (Reading-in of the schemas in examply.py and schema.py would be done in settings.py or an extra module next to it).

@4lm 4lm marked this pull request as draft October 10, 2022 09:39
@4lm
Copy link
Contributor Author

4lm commented Oct 10, 2022

Hi @henhuy,

Thank you for your review! Following, I will address your points:

Does the schema for a single results (single municipality) looks different from current example.json? Or simply just have a single entry in series (aka len(series) == 1)? ;)

Yes, the schema is the same. You just have a single entry in a series in the case of one municipality. The example.json is shortened. Typically, two cases exist: (i) a list of one municipality or (ii) a list of twenty municipalities.

Do schemas for other popups differ from example.json in population folder? Or can we agree on a default schema for all results? Or maybe a default schema, and differing schemas in case the schema does not fit for a special result? This would make development of the API much easier...

I'm shooting for "a default schema and differing schemas in case the schema does not fit for a special result." Still, I can't guarantee that the proposed schema will change until then. Suggestion, I will add a default schema to this PR that I extract from the population schema (which I already created with generic use in mind). Let us have 2 - 3 more implementations and finalize it on the fly. For this, we need actual data! @nesnoj, wink wink 😉

If we agree on a default schema, I would move up the schema.json one folder into folder schema.

OK, I will move it.

I would like to remove init.py from metadata folders and keep it as a non-python/config-like structure (Reading-in of the schemas in examply.py and schema.py would be done in settings.py or an extra module next to it).

OK, I will make the appropriate changes.

@4lm 4lm marked this pull request as ready for review October 10, 2022 12:52
@4lm
Copy link
Contributor Author

4lm commented Oct 10, 2022

@henhuy, finally ready 😃 after a little fight with the linter, please review the changes I made

@4lm
Copy link
Contributor Author

4lm commented Oct 10, 2022

I made everything more generic and included chartType and sources attributes. And moved everything to the demanded folders/places.

@4lm 4lm marked this pull request as draft October 10, 2022 14:57
@4lm
Copy link
Contributor Author

4lm commented Oct 10, 2022

I switched the PR to draft mode. As decided in the weekly, I will add atomic component schemas (e.g., charts) before asking for another review.

@4lm 4lm marked this pull request as ready for review October 10, 2022 17:59
@4lm
Copy link
Contributor Author

4lm commented Oct 10, 2022

@henhuy, I extracted an atomic chart schema and example and referenced it in the default schema. Please review this PR and merge on approval.

Please note I mocked the test because the $id properties of the schemas have to be proper and existing URIs. This is not the case yet. Right now, they point to non-existing endpoints of: https://wam.rl-institut.de/digiplan/schemas/[...]

@4lm 4lm mentioned this pull request Oct 12, 2022
@4lm
Copy link
Contributor Author

4lm commented Oct 12, 2022

Again setting this PR to draft because I'm implementing in #73 the popup on map an realized I can do the format even more generic - stay tuned :)

@4lm 4lm marked this pull request as draft October 12, 2022 17:16
@4lm 4lm marked this pull request as ready for review October 13, 2022 20:08
@4lm
Copy link
Contributor Author

4lm commented Oct 13, 2022

@henhuy, I finalized the schemas :) Please review and merge if approved!

Now we have component schemas (chart and sources) and one specific schema (popup). I aim for generic components and individual schemas for specific implementations with their own specific constraints (like the popup schema has). I think this gives it a good balance between complexity and reusability. I also sliced off anything that is not absolutely needed for the client in the API response object (and for you to implement in the backend).

Having a set of components will help us to construct the rest of the schemas in a faster and more generic way. However, I gave up trying to do everything generic because I realized this didn't work well. For example, the popup has specific constraints that cannot be adequately generic. And if you try to do everything generic, then I would have to define informal business logic, which constructs the appropriate data structures, in the frontend in JavaScript. This, in my opinion, is not a good solution and would make everything super messy and is the opposite of our rule for good web app architecture: "dumb frontend, intelligent backend."

Therefore I vote for generic components and schemas for specific API endpoint implementations for particular needs (e.g., map popup, result page with charts, etc.). Ultimately, I think there will only be a handful of specific schemas (maybe 3 - 4). The first one is the popup schema given in this PR.

4lm added a commit that referenced this pull request Oct 13, 2022
@4lm
Copy link
Contributor Author

4lm commented Oct 13, 2022

@henhuy, please also have a look at the JSON API placeholder popup.json file in #73 which might help you to better understand the use of $ref in the JSON schema of this PR.

@henhuy
Copy link
Collaborator

henhuy commented Oct 17, 2022

Looks very nice! This is how I imagined it. Thank you very much

@henhuy henhuy merged commit 7268a4f into dev Oct 17, 2022
@henhuy henhuy deleted the add-metadata-schema-and-tests branch October 17, 2022 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants